fix(registration): send the response ttl as a number - #138
Merged
Conversation
It was the string '300', the only ttl this API sends that was not a number. A caller sets its registration cookie from it, and the Fastify adapter passes the value to a cookie library that requires an integer, so registration failed there with `TypeError: option maxAge is invalid: 300`. The Express adapter multiplies it into milliseconds, which coerces the string, so the same response worked and the mismatch went unnoticed. Response bodies are validated against the shared schema at runtime, so this needs @seamless-auth/types 0.6.0, where RegistrationSuccessSchema.ttl becomes a number. The dependency pin is bumped separately once that release lands. Verified with npm run typecheck, npm run test:run (91 files passing), npm run lint, and npm run format:check, against a local build of the types package. openapi.json and src/generated/api.ts regenerated; the contract test enforces it.
The release where RegistrationSuccessSchema.ttl is a number. Response bodies are validated against that schema at runtime, so the registration change on this branch returned 500 against 0.5.0 and only works from here. Verified against the published package with npm run typecheck, npm run test:run (91 files passing), npm run lint, and npm run format:check. Re-running npm run generate:api produces no diff, so the committed contract already matches the published schema.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second of three. Draft: blocked on fells-code/seamless-auth-types#27.
Response bodies are validated against
RegistrationSuccessSchemaat runtime, so with the publishedtypes@0.5.0(which still declaresttl: z.string()) this change makes registration return 500. The test suite catches that, which is how I confirmed the coupling. Verified against a local build of the fixed types package. The dependency bump is the last commit here before it comes out of draft.The bug
Registration fails on the Fastify adapter with
TypeError: option maxAge is invalid: 300, because this API sendsttl: '300'as a string andcookierequiresNumber.isInteger.It was the only
ttlthis API sends that was not a number. The Express adapter multiplies it into milliseconds, which coerces the string, so the same response has always worked there and the mismatch went unnoticed.The change
ttl: '300'becomesttl: 300, with a comment recording the invariant that it has to match the5minsignEphemeralToken. A caller sets its registration cookie from this value, so a larger number here would leave a cookie outliving the token it carries.openapi.jsonandsrc/generated/api.tsregenerated:"ttl": { "type": "string" }becomes"type": "number". The contract test enforces it.Scope I backed out
I first hoisted the lifetime into an exported
EPHEMERAL_TOKEN_TTL_SECONDSso the5mand the300collapsed into one value. Four test files mocksrc/lib/token.js, and each would have needed the new export added to its mock. That is a lot of test-infrastructure churn for what is a one-word type fix, so I reverted it and left the invariant as a comment.Worth doing on its own: the two literals can still drift, and a comment is weaker than a shared constant. Happy to open it separately.
Verification
npm run typecheck,npm run test:run(91 files, all passing),npm run lint,npm run format:checkall clean, against a local build of the types fix.